Conversation
…avoid needing them to go through a global location (which would require some complexity to handle right)
|
Added a refactoring to give workers a direct pointer to the pool, avoiding them using a global variable which was less nice. This should also fix an issue noticed about destruction order of that global in #1376 |
There was a problem hiding this comment.
So this prevents issues where you want to use the global pool to do work from different modules (which are created from different threads?) It would be really useful (both for review and for future archaeology) to put all of that in the commit message (as opposed to just linking to the issue).
I think I understand this and it looks to me like it will do what you want.
I would actually use a different architecture though, where there is no global state; the Binaryen module is essentially an isolate that everything hangs off of (even the thread pool). This means of course that there could be more than one thread pool in a process, if the user creates multiple Binaryen modules. But it does have the advantage that it makes it much easier for users/callers to reason about the behavior, and everything is under their control, rather than having complex (and potentially surprising) serializing behavior hidden from the users. We should probably allow the user to control the size of the thread pool and/or whether it is used at all; then they can then decide whether they care about serializing the work of the thread pools, etc. And if the default is to use "all" the cores, then the failure mode would just be that the cores would be oversubscribed.
| '-Isrc', '-g', '-L' + libdir, '-pthread'] | ||
| print 'build: ', ' '.join(extra) | ||
| if src.endswith('.cpp'): | ||
| extra += ['-std=c++11'] |
There was a problem hiding this comment.
I thought clang already defaulted to C++11?
(actually I think its gnu++11, but that doesn't matter, does it?)
There was a problem hiding this comment.
Depends on the version of clang and gcc. When I run locally in Ubuntu LTS using gcc, it needs that flag.
|
Yeah, I'll document this better in the final commit here, good point. About the threadPool being global or not, there are advantages both ways, yeah. I prefer a single global one for a library like binaryen, since threads are a single global resource in a sense. Even if you create multiple modules and optimize them in parallel, you usually only want a number of threads active that is equal to the number of cores, and not potentially the number of cores times the number of modules. |
|
It seems much of the complexity here comes from having |
|
True, but after this PR I think most of that complexity is gone. The one place it's left is that we lock in the |
|
Let's merge this in, as it is confirmed to fix all known problems here. We can do further improvements later. |
3 participants
More fixes for #1376
Includes a proper test, which did not pass before and now does.